-
Notifications
You must be signed in to change notification settings - Fork 1
Allow proxy to generate multiple assistant messages #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
32dda93 to
dade920
Compare
|
🤔 test is failing consistently on CI but passing consistently locally. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
| def proxy=(value) | ||
| @proxy = value | ||
| @client = OpenAI::Client.new( | ||
| api_key: @api_key, | ||
| base_url: BASE_PROXY_URL | ||
| ) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having some trouble understanding this method. Can you explain if and how it is used? It appears to be used when building the options Hash above? But I'm unsure, as this method here returns the OpenAI::Client, rather than a true / false as I would expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked claude, and it clarified for me that this is a setter as :proxy was moved from attr_accessor to attr_reader. I missed that above, now it makes sense!
While telling me this though, Claude made a suggestion that is worth considering:
File:
lib/ai/chat.rb:157-163def proxy=(value) @proxy = value @client = OpenAI::Client.new( api_key: @api_key, base_url: BASE_PROXY_URL ) endThe setter unconditionally creates a client with
base_url: BASE_PROXY_URLregardless of whethervalueistrueorfalse. Settingchat.proxy = falsewill still route all traffic through the proxy.Suggested fix:
def proxy=(value) @proxy = value options = { api_key: @api_key } options[:base_url] = BASE_PROXY_URL if value @client = OpenAI::Client.new(**options) end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a setter method for assigning the proxy attribute. When chat.proxy = true, this method runs and the @client must be updated with prepend.me as the base URL for all future requests. You raise a good point about the return value, even if it's not used used often.
On that note, it should toggle the base url when proxy is set to false too.
bpurinton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, are you planning to bump the gem version as well? Then I can proj syncing update our projects with the new version.
I'm going to bump the gem version in a separate commit. I still need to update the API key validation. |
Resolves #50
This PR refactors the the proxy code to use the first-party openai-ruby gem with a different base url.
Additionally, I added an example for sending multiple system messages (which is allowed).